fix(gin): literal colon routes not working with engine.Handler()#4415
fix(gin): literal colon routes not working with engine.Handler()#4415appleboy merged 7 commits intogin-gonic:masterfrom
Conversation
…teral colon routes in all usage scenarios (#4413)
There was a problem hiding this comment.
Pull Request Overview
This PR implements lazy initialization of route trees using sync.Once to ensure the updateRouteTrees() function is called exactly once when routes are first accessed, rather than being called explicitly in every Run* method. This approach handles literal colons in route paths (escaped as \:) more consistently across different server startup patterns.
- Added
routeTreesUpdated sync.Oncefield to Engine struct to ensure single initialization - Modified
ServeHTTPto callupdateRouteTrees()lazily on first request usingsync.Once - Added comprehensive tests covering literal colon handling in various scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gin.go | Added routeTreesUpdated sync.Once field and modified ServeHTTP to lazily initialize route trees on first request |
| literal_colon_test.go | Added test suite covering literal colon functionality with different server initialization patterns and testing that route tree update happens only once |
Comments suppressed due to low confidence (1)
gin.go:542
- The
Run()method still explicitly callsupdateRouteTrees()before starting the server. With the new lazy initialization inServeHTTP, this meansupdateRouteTrees()could be called twice: once here and once by thesync.OnceinServeHTTP. SinceRun()callsengine.Handler()which returns the engine itself (when UseH2C is false), requests will trigger thesync.Oncecall. This creates inconsistent behavior whereRun()eagerly updates but directServeHTTPusage updates lazily. Consider usingengine.routeTreesUpdated.Do(func() { engine.updateRouteTrees() })here instead.
engine.updateRouteTrees()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // routeTreesUpdated ensures that the initialization or update of the route trees | ||
| // (used for routing HTTP requests) happens only once, even if called multiple times | ||
| // concurrently. It helps prevent race conditions and redundant setup operations. |
There was a problem hiding this comment.
The documentation states this prevents race conditions, but sync.Once alone doesn't prevent race conditions on the route trees themselves - it only ensures the function is called once. If routes can be added after the first ServeHTTP call, there could still be concurrent access issues. Consider clarifying that this ensures initialization happens once, and note any assumptions about route registration happening before serving requests.
| // routeTreesUpdated ensures that the initialization or update of the route trees | |
| // (used for routing HTTP requests) happens only once, even if called multiple times | |
| // concurrently. It helps prevent race conditions and redundant setup operations. | |
| // routeTreesUpdated ensures that initialization of the route trees (used for routing HTTP requests) | |
| // happens only once, even if called multiple times concurrently. Note: sync.Once only ensures | |
| // the initialization function is called once; it does not prevent race conditions on the route trees | |
| // themselves if routes are added after the first ServeHTTP call. It is assumed that all route | |
| // registration happens before serving requests to avoid concurrent access issues. |
There was a problem hiding this comment.
sync.OnceensuresupdateRouteTrees()is called exactly once, not that it prevents all race conditions.- This implementation assumes routes are registered before serving begins, which is consistent with Gin's existing design. The
engine.Run()method already callsupdateRouteTrees()once with this same assumption - this PR simply extends that pattern toHandler()and directhttp.Handlerusage. - Adding routes dynamically after serving begins is not supported in Gin's current architecture (this is existing behavior, not introduced by this change).
There was a problem hiding this comment.
updated the comments for routeTreesUpdated in Engine interface
|
Hi @appleboy , The golangci-lint was failing, I used |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
gin.go:511
- Corrected grammar: 'do update to' should be 'does update to' or 'updates'.
// updateRouteTrees do update to the route trees
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, http.StatusOK, w.Code) | ||
| } | ||
|
|
||
| _ = originalUpdate |
There was a problem hiding this comment.
This blank identifier assignment serves no purpose and suggests incomplete test implementation. Either remove this line or implement the intended verification logic for the test.
| _ = originalUpdate |
There was a problem hiding this comment.
This is a test case that checks whether updateRouteTrees is called only once.
There was a problem hiding this comment.
updated the test case
|
Hi @pawannn, thanks for your great work on this! When I initially opened the issue, I was hoping it might lead to a discussion on potential optimizations, such as the placement of sync.Once in hot paths, but your current solution is also quite good. I noticed some review comments and took the liberty of making a few small iterations on top of your commit to clean up the code and improve the tests. You can find my changes in my repository: Zhang-Siyang/gin@96b7c64...54a1967 Feel free to take a look and see if any of these changes are helpful for this PR. Looking forward to hearing from you! |
|
Hey @appleboy, apologies for the formatting issue. I've run
|
|
Hey @Zhang-Siyang ! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4415 +/- ##
==========================================
- Coverage 99.21% 98.83% -0.38%
==========================================
Files 42 44 +2
Lines 3182 2924 -258
==========================================
- Hits 3157 2890 -267
- Misses 17 21 +4
- Partials 8 13 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|

Problem
Literal colon routes (e.g.,
/api:v1) currently fail when using:engine.Handler()http.Handlerusage (e.g.,&http.Server{Handler: engine})They only work when using engine.Run()
Related
Reproduction
Output Before Fix
Root Cause
The
updateRouteTrees()method converts escaped paths (/api\:v1) to actual colon paths (/api:v1), but it's only called insideengine.Run(). Other usage patterns never trigger this conversion.Fixes #4413
Solution
Add a
sync.Oncemechanism inServeHTTP()to ensureupdateRouteTrees()is called exactly once before processing the first request, regardless of how the engine is used.Changes
Modified Files:
gin.go: AddedrouteTreesUpdated sync.Oncefield to Engine struct.gin.go: UpdatedServeHTTP()to callupdateRouteTrees()once on first request.Testing
engine.Run()engine.Handler()ServeHTTP()usageupdateRouteTrees()is called only once across multiple requestsOutput after the Fix:
Result: Request to
/api:v1now returns 200 OK with the correct response!Breaking Changes
None. This is a pure bug fix with no API changes.
Pull Request Checklist
Please ensure your pull request meets the following requirements:
masterbranch.Thank you for contributing!